Conversation
lithammer
requested review from
TSonono,
TobbeEdgeware,
lithammer and
nilsandgren
|
It would be nice to hear your views before I PR to grafov |
TobbeEdgeware
left a comment
TobbeEdgeware
left a comment
There was a problem hiding this comment.
This is way too many changes and to just add CHANNELS to the master playlist.
Especially if this should be submitted as a PR it must be focused on the one thing that it addresses and make it without big changes to any files.
| IETF drafts notes | ||
| ----------------- | ||
|
|
||
| [IETF](http://ietf.org) document currently in Draft status. Different versions of the document introduce changes of HLS protocol playlist formats. Latest version of the HLS protocol is version 7. |
There was a problem hiding this comment.
Why do this crappy updates on HLS version which has nothing to do with Channels?
If you do this, do this properly. The latest draft of HLS is https://datatracker.ietf.org/doc/html/draft-pantos-hls-rfc8216bis-10 and the latest version of HLS is 10.
I'd suggest to just drop this and the next changes to the repo.
| case "CHANNELS": | ||
| channels, err := strconv.Atoi(v) | ||
| if err != nil { | ||
| return fmt.Errorf("non-integer value \"%s\" for CHANNELS attribute", v) |
| p.buf.WriteRune('"') | ||
| } | ||
| p.buf.WriteRune('\n') | ||
| if alt.Channels != 0 { |
There was a problem hiding this comment.
This must go before the newline just above.
| @@ -0,0 +1,152 @@ | |||
| #EXTM3U | |||
There was a problem hiding this comment.
why this extremely long list to check for one simple attribute?
And why remove media-playlist-large and add media-playlist-long instead?
| } | ||
| } | ||
|
|
||
| func TestDecodeMediaPlaylistLong(t *testing.T) { |
There was a problem hiding this comment.
What does this test have to do with channels?
There was a problem hiding this comment.
Nothing, technically, but:
the other Channels-PR grafov#168 had an increase of the Playlist size. I was making sure that was not necessary
megaadam
force-pushed
the
support-for-CHANNELS
branch
from
494c07e to
610edb6
Compare
|
I should also add |
2 participants
@TobbeEdgeware @TSonono @nilsandgren @lithammer
Enjoy!
It seems after all that you are supposed merge upstream from your own fork.